#22411 - Enforce token write ability when running scripts via the REST API#22412
#22411 - Enforce token write ability when running scripts via the REST API#22412arthanson wants to merge 3 commits into
Conversation
|
@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration. |
|
Claude finished @arthanson's task in 2m 56s —— View job Code Review
Two items worth noting: 1. Write-check placed after script lookup (minor info leak) In # Current order:
script = self._get_script(pk) # 404 if not found
if isinstance(request.auth, Token) and not request.auth.write_enabled: # 403 after
raise PermissionDenied(...)
# Safer order:
if isinstance(request.auth, Token) and not request.auth.write_enabled: # 403 first
raise PermissionDenied(...)
script = self._get_script(pk)2. Inconsistency with
No other concerns. The fix is correct, the import path matches |
|
Per claude review, addressed 1, 2 is a direct consequence of these changes - collapsing them back to one pattern would reintroduce the bug. |
jeremystretch
left a comment
jeremystretch
left a comment
There was a problem hiding this comment.
While I agree generally with the assertion that executing a custom script should require write permissions, this would be a breaking change. IMO we should target it for NetBox v4.7. (Consider a scenario where a read-only API token is used to execute a custom script that performs only read operations, e.g. to generate some report.)
|
rebased to feature branch |
2 participants
Closes: #22411
The script execution endpoint (
POST /api/extras/scripts/{id}/) did not honor the calling token'swrite_enabledflag.ScriptViewSetoverridespermission_classeswith[IsAuthenticatedOrLoginNotRequired], which removesTokenPermissions(the only permission class that consultsrequest.auth.write_enabled) from the chain. As a result, a read-only token (write_enabled=False) whose user holds therun_scriptpermission could still trigger script execution, bypassing the read-only restriction that operators expect such tokens to enforce.Changes
ScriptViewSet.post(): when the request is token-authenticated (request.authis aToken) and the token haswrite_enabled=False, the request is rejected with403 Forbidden. The existing object-levelrun_scriptpermission check is retained.IsAuthenticatedOrLoginNotRequired, so session-authenticated requests (whererequest.authis not aToken) are unaffected and continue to work as before.ScriptTestCase.test_run_token_write_enabled: awrite_enabled=Falsetoken (with therun_scriptpermission) receives403 Forbidden, and200 OKonce the token's write ability is enabled.ScriptTestCase.test_run_session_auth: a session-authenticated request (no token) is still permitted to run scripts, guarding against a regression in the session-auth path.Notes
The other REST API viewsets that override
permission_classeswithIsAuthenticatedOrLoginNotRequired(StatusView,ObjectTypeViewSet,ConnectedDeviceViewSet) expose only read-only (GET) methods, so the missing write check has no effect there —ScriptViewSetwas the sole affected endpoint.